[PM-41954] fix(desktop): isolate native messaging per browser - #22409
[PM-41954] fix(desktop): isolate native messaging per browser#22409Donnerbart wants to merge 1 commit into
Conversation
|
Thank you for your contribution! We've added this to our internal Community PR board for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
12eb80c to
39b6f23
Compare
|
Only another user here.
FYI, I think #20148 was superseded by #21784 [EDIT: I was wrong here - and corrected that further down] (and I guess there were other changes too, also in regards to "Shared unlock", mostly done by @quexten) |
|
Hmm, the verb superseded throws me off a bit. #20148 is an issue, #21784 is a code change. Normally a code change would supersede another code change and close/fix an issue, not supersede an issue. In the end this doesn't matter. I was referencing issues and code changes with similar symptoms and partial fixes for context and history of the current code. The issues I experienced match #12697 the best, which I hope to fix with this PR. And yes, I ran into this while setting up "shared unlock". |
generateManifests() aborted on the first browser that threw, and main.ts calls it immediately before listen(). One failing browser therefore prevented the IPC server from starting, disabling native messaging for every browser rather than just the one that failed. Create the native messaging hosts directory for all browsers instead of Firefox only, and give each browser its own try/catch so a failure is logged and skipped. Drop the caller-side "Hard-linked" log lines. linkOrCopy already logs whether it linked or fell back to copying, so the extra line reported a hard link even when the file had been copied. Fixes bitwarden#12697
39b6f23 to
09d14f8
Compare
Ah, Sorry, my mistake. I did not realize you were quoting an issue with #20148. In fact, there were a few issues around that topic - and I think I confused this issue with #21141 (and I think that was superseded by #21784). But anyway, I think BW has this issue on their radar... I'm looking forward to a (any) solution at all.
Probably not, since "shared unlock" is not released yet. (unless you tried to compile future app versions on your own) |
Probably totally irrelevant for that PR now (and I probably while hide the comment later), but
|
Oh, but I am, and there is no compilation needed. The code is already released, you just need to set two feature flags in the app and the extension to activate it. With those configured the IPC server is started in the desktop app, so then I ran into this issue. You'd probably also run into this if you try to use the biometric unlock from the desktop app (should be the same connection being used).
This is not relevant, since this is client-only feature. Self-hosted vaultwarden, newest version. |
@Donnerbart On which chromium based browser are you seeing this? On fedora the NMHS dir seems to be created by chromium automatically. The changes seem reasonable to me. |
The first folder that caused problems for me was I assume some of these folders might be created by 1Password (which I use for work), e.g.: I also saw a GS Connect file somewhere, for another browser I don't use. So I think all these different states of folders that might already exist or not, are caused my multiple software products, setting up their files for native messaging. Combine that with an uninstall and incomplete cleanup, and you quickly end up with a wild mix of On top I had the write permission error on some of these config folders due to the snap sandbox of the bitwarden app (I switched to the flatpack installer to work around that). That probably made it hard to find the one root cause for all of these issues. |
4 participants
🎟️ Tracking
Fixes #12697
Follows up on #20148, which added directory creation for Firefox only, and #21783, which made the DuckDuckGo manifest step non-fatal.
📔 Objective
generateManifests()aborts on the first browser that throws, andmain.tscalls it immediately beforenativeMessagingMain.listen(). One failing browser therefore stops the IPC server from ever starting, disabling native messaging for every browser rather than just the one that failed. The only trace is a single line inapp.log; in the app the symptom is that browser integration and biometric unlock silently do not work.Two causes observed on Linux:
NativeMessagingHostssubdirectory.mkdirran for Firefox only, solinkOrCopythrewENOENTfor chromium-family browsers. This is the same report as Browser integration with Firefox is not enabled until manually creating the NativeMessagingHosts directory #20148, one browser over, and matches the manual workaround people describe in Can not enable system authentication(unlock with biometric) on bitwarden extension #12697.EACCES.In both cases the browser at fault was one the reporter had never used with Bitwarden.
Whether the feature breaks partially or completely depends on the entry point, because the two call sites order the operations oppositely. Toggling browser integration in settings runs
listen()first, so the socket survives the throw and browsers earlier in the iteration order keep working, which is why Firefox is often the only one that still works. On app startup the throw happens beforelisten(), so no socket is created and no browser works.Changes
try/catch, so a failure is logged and skipped instead of aborting the remaining browsers andlisten().linkOrCopyalready logs whether it linked or fell back to copying, so the extra line reported a hard link even when the file had been copied.Note for reviewers
The directory is now created for chromium-family browsers where it previously was not, so the proxy binary gets copied into browser config directories that exist but have no Bitwarden integration set up yet. That is already how Firefox behaves, and the guard is unchanged in that the browser's own config directory must already exist, but it is a behavior change worth calling out.
Logging stays at
errorfor a skipped browser. This condition already surfaced aslogService.error("Error while setting up native messaging:", err)frommain.ts, so keeping the level avoids making it harder to find in logs than it is today.Added
native-messaging.main.spec.tscovering both behaviors. Both tests fail against the unmodified source.